-
Notifications
You must be signed in to change notification settings - Fork 782
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update devp2p API names and access specifiers #2889
Conversation
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
…IdFilter, and _capabilities
… _refillIntervalId, and _refillIntervalSelectionCounter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super cool, going in a good direction 👍 and this will so dramatically improve the API (e.g. for code completion) of this package 🙏, phew, wasn’t aware that there are SO many properties floating around. 😅
…ethereumjs/ethereumjs-monorepo into devp2p-fields-access-and-naming-cleanup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏 🙏 🙏
Absolutely love this! 😍
Super clean and consistent, thanks a lot for this straight forward tackling!
Will merge.
protected _requests: Map<string, any> | ||
protected _requestsCache: LRUCache<string, Promise<any>> | ||
protected _socket: DgramSocket | null | ||
private _debug: Debugger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likely not practically relevant and only a small nit, but I would think that it would make sense to just not distinguish between "even more internal" or so variables and just also make _debug
protected, maybe someone wants to customize the devp2p package and add own debug messages, whatever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was on the fence about this one, but since debuggers are usually initialized specific to a class, I opted for private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The protected
version is also remaining in the class scope, so this is just a bit broader so that people can e.g. subclass the Server
class, e.g. for a custom binance devp2p implementation (super random example) or whatever. And then on the subclass one might want to adjust some properties and it is just a bit unlucky if these are then not accessible. So at some point in time [tm] we generally decided to favor protected
over private
, since it (more or less) achieves the same thing and has fewer side effects.
This PR addresses part of #2872 by renaming and rethinking access specifiers for properties of devp2p package classes.